Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid panic on shut down when TLS configuration is present #8986

Closed
wants to merge 3 commits into from

Conversation

jamesdphillips
Copy link
Contributor

Fixes #8916

With regards to the panic gRPC team suggests:

There are a number of features that are not supported or available when serving via ServeHTTP(), and this is one of them.

The implementation for Drain() in grpc's transport sends a GOAWAY to the client. In a multitenancy environment (ServeHTTP()), this would be inappropriate (if it is even possible), as it would affect the other handlers. The gRPC spec has no other way to notify the client that it should stop sending new RPCs.

The right way to do what you want is to perform a graceful stop on the http server (using Shutdown) instead of the gRPC server.

Full comment can be found here.

Hope fix is satisfactory; happy to refactor if need be. 👍

Avoid panic when stopping gRPC Server if TLS configuration is present.
Provided solution (attempts to) implement suggestion from gRPC team: grpc/grpc-go#1384 (comment).

Fixes etcd-io#8916
embed/serve.go Outdated
}

// Gracefully shutdown gRPC server when using HTTP2 transport.
func stopInsecureServer(gs *grpc.Server, timeout time.Duration) {
Copy link
Contributor

@gyuho gyuho Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gyuho gyuho self-assigned this Dec 7, 2017
@gyuho
Copy link
Contributor

gyuho commented Dec 7, 2017

@jamesdphillips Approach looks good. Let me refactor this branch further, with additional tests around it. Thanks for contribution!

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
@@ -82,7 +82,7 @@ type Etcd struct {
type peerListener struct {
net.Listener
serve func() error
close func(context.Context) error
close func(time.Duration) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why do we need this change. can you explain?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous (e *Etcd) stopGRPCServer(gs *grpc.Server) method was using embed.Etcd only to compute request timeout. Now, we want to use this same function with *http.Server parameter. Especially, in line https://github.com/coreos/etcd/pull/8986/files#diff-d7a47eb75475dba263540eb7f8456e50R258.

If we have time.Duration as an argument, it's easier to use this function for all shutdown calls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http server shutdown takes a context: https://golang.org/pkg/net/http/#Server.Shutdown

we should do the same, no?

@gyuho
Copy link
Contributor

gyuho commented Dec 7, 2017

Semaphore is not working in this branch for some reason https://semaphoreci.com/coreos/etcd/branches/pull-request-8986/builds/4.

Moving to #8987.

@gyuho gyuho closed this Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Panic when stopping etcd process
3 participants